Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Gluster version to 7.9 #232

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Conversation

tsmgeek
Copy link

@tsmgeek tsmgeek commented Jul 23, 2021

Update default version from 7.3 to 7.9 the last in the 7.x line.

Updated documents to match default version.

@tsmgeek
Copy link
Author

tsmgeek commented Sep 7, 2021

@rwaffen can we push release to 7.9 and also update forge

Copy link
Member

@attachmentgenie attachmentgenie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsmgeek you have missed one reference on line 66 of the readme. we mention the 3.8 release being installed

Copy link
Member

@attachmentgenie attachmentgenie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@smortex why do you consider this a BC break, seems a simple upgrade of the same major release to me

@smortex
Copy link
Member

smortex commented Sep 14, 2021

@attachmentgenie since the default value of a parameter is changed, it will have consequences with users who rely on this default 🤷

Another way to deal with this and avoid installing an outdated version is to not set a default and make it mandatory for the user to set it explicitly. I am not a user of this module, so I let actual maintainers decide what they want to do: keep the version the way it is, update it or change the way it is managed 😉

@tsmgeek
Copy link
Author

tsmgeek commented Sep 14, 2021

But does that not depend if they are pinning the puppet version or pulling from git directly.

It's a byproduct of not updating puppet version or a gittag for so long that people ended up pulling the master branch.

Ide say that as its master it should not matter what is done, only the puppet release version is important.

Copy link
Member

@attachmentgenie attachmentgenie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tsmgeek if you could squash the commits into one, than i am happy to approve this one

Update all references to 7.9
@tsmgeek
Copy link
Author

tsmgeek commented Sep 16, 2021

Squashed.

Proposing that a minor gluster release (or any minor fixes to package) only changes patch level of puppet release (6.0.1) but if there is a move from 7->8 we go to 6.1.0. Does that seem reasonable?

If no one objects to #235 I am planning to push forward for a 7->9 PR so we bring this up to date,

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proposing that a minor gluster release (or any minor fixes to package) only changes patch level of puppet release (6.0.1) but if there is a move from 7->8 we go to 6.1.0. Does that seem reasonable?

Changing a patch version should only fix bugs. (yes, we try to follow semver even if it is designed for versionning API and a Puppet module is way more than an API):

Patch version Z (x.y.Z | x > 0) MUST be incremented if only backwards compatible bug fixes are introduced. A bug fix is defined as an internal change that fixes incorrect behavior.

So if we change the default for a parameter this is not a bug fix, not a new features users can rely on, and definitively a breaking change. But it's not an issue to roll a new major version: it's the same process for patch, minor and major so 🤷

If no one objects to #235 I am planning to push forward for a 7->9 PR so we bring this up to date,

Yes, if version 7 is deprecated, using this as a default does not make sense.

If multiple versions are supported, it may make sense to list them in a enum and test them all in CI, but it is another story 😃

@vox-pupuli-tasks
Copy link

Dear @tsmgeek, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@tsmgeek tsmgeek mentioned this pull request Sep 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants